Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a belongs_to record from the associated record screen #1876

Merged
merged 20 commits into from
Oct 20, 2023

Conversation

sdcoffey
Copy link
Contributor

@sdcoffey sdcoffey commented Jul 31, 2023

Description

Allows creation of a belongs_to record from the new or edit pages of a resource.

Accomplished this by:

  1. Adding a link to Create new x below the belongs_to field. This link points to the associated resource's new view with a special url param :via_belongs_to_resource_class.
  2. Modifying BaseController#new to render a turbo stream adding a modal to the page containing the associated resource's new view, when the above param is present.
  3. Modifying BaseController#create_success_action to, when the above param is present, render two turbo streams, one removing the modal, and the other containing a custom event with the details of the new record.
  4. Adding a new stimulus controller to handle the custom event, updating the field with the params from the newly created record. Aside: although this introduces some bespoke js, it turned out to be way less intrusive than some other, non client-side approaches I tried.

Fixes #1556

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works

Manual review steps

  1. Create a Course from edit/new of Course::Link - searchable belongs_to association
  2. Create a User from edit/new of Fish - belongs_to with normal HTML select
  3. Create a Post or Project from new/edit of Comment - polymorphic belongs_to
  4. Create a Fish from new/edit of Review - searchable, polymorphic belongs_to

Manual reviewer: please leave a comment with output from the test if that's the case.

Open Questions

  1. Should there be a field-level option to enable/disable this behavior?
  2. How should we handle cases where the resource pointed to is an abbreviated version of the associated resource (e.g. CompactUser)?

@codeclimate
Copy link

codeclimate bot commented Jul 31, 2023

Code Climate has analyzed commit 528432c and detected 0 issues on this pull request.

View more on Code Climate.

@sdcoffey sdcoffey changed the title 🚧 WIP - Create a belongs_to record from the associated record screen Create a belongs_to record from the associated record screen Jul 31, 2023
@github-actions
Copy link
Contributor

This PR has been marked as stale because there was no activity for the past 15 days.

@adrianthedev
Copy link
Collaborator

@Paul-Bob can you please also have a look over this when you get a chance? Thank you.

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Aug 23, 2023

Hi @sdcoffey, first of all thank you for the amazing PR.

I've been testing it and came across a few issues. The modal that opens for the belongs_to creation seems a little unpolished, for example, when creating an user or some record that have a big resource it expands to full height, maybe we should assure some margins. It's no biggie.

My main concern revolves around the form errors. They no longer seem to show up when submitting a form and some validation fails. It happens due turbo-stream I think. We should focus on solving this error rendering issue first.

Otherwise, the functionality seems to be there, good job!

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's assure that errors render on the form when validations fails

Comment on lines 1 to 3
<%= turbo_stream.append "alerts" do %>
<%= render Avo::FlashAlertsComponent.new flashes: flash %>
<% end %>
Copy link
Contributor

@Paul-Bob Paul-Bob Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should rename this stream to .../_flash_alerts.turbo_stream.erb I think it fits better to the file porpoise.

@sdcoffey
Copy link
Contributor Author

Thanks for the review @Paul-Bob! Will take another look at the issue with large resources and fix the missing validation errors (totally missed that).

Will ping you again when I've finished updating

@sdcoffey
Copy link
Contributor Author

Hey all! Been a couple weeks since the last activity, wanted to bump this to see about getting it across the finish line

@adrianthedev
Copy link
Collaborator

Yes @sdcoffey. You're right! It's on me to review it.
Sorry about the delay.

I'll give it a go tomorrow.

@sdcoffey
Copy link
Contributor Author

No worries, just wanted to make sure it didn't get lost–I'm sure y'all are busy! Thank you! 🙏

@adrianthedev
Copy link
Collaborator

@sdcoffey it works great but I need to check the code a bit and the styling. It's on my plate for the weekend.

@sdcoffey
Copy link
Contributor Author

Sounds good! Thank for the update

@adrianthedev adrianthedev merged commit 11aae5c into avo-hq:main Oct 20, 2023
10 checks passed
@adrianthedev
Copy link
Collaborator

Thanks @sdcoffey for the contribution.
Sorry it took so long to get it merged.

@sdcoffey
Copy link
Contributor Author

🙏 No worries! Thanks for the cleanup.

Excited to see this make it into the next release!

@adrianthedev
Copy link
Collaborator

Yes. It will be out on Tuesday 🙌

</div>
<% end %>

<% if content? %>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this content? call is causing a NoMethodError

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@segiddins can you describe the scenario? I wasn't able to reproduce it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubygems/rubygems.org#4154 yeah, tests started failing on the version bump

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might need to update view_component on your end.
I can't put my finger on it exactly, but at some point they changed the way they figure out if the slot has any content inside.

Copy link
Collaborator

@adrianthedev adrianthedev Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that view_component v3 added this helper. Are you able to update to VC 3 @segiddins?
I see that you are already on 2.8+ which should not bring breaking changes AFAIK.

@xeron
Copy link

xeron commented Oct 25, 2023

I'm getting a error trying to use new resource links introduced by this PR.

e.g. this URL /admin/resources/drivers/new?via_belongs_to_resource_class=ResultResource&via_relation=driver gives me a error:

No route matches {:action=>"new", :controller=>"avo/drivers"}

but this works just fine /admin/resources/drivers/new.

Trace example:

Started GET "/admin/resources/drivers/new?via_belongs_to_resource_class=ResultResource&via_relation=driver" for 127.0.0.1 at 2023-10-24 20:48:43 -0700
Processing by Avo::DriversController#new as HTML
  Parameters: {"via_belongs_to_resource_class"=>"ResultResource", "via_relation"=>"driver"}
  Rendered vendor/bundle/ruby/3.2.0/gems/avo-2.43.0/app/views/avo/base/_new_via_belongs_to.html.erb (Duration: 13.3ms | Allocations: 8543)
Completed 500 Internal Server Error in 18ms (ActiveRecord: 0.0ms | Allocations: 10221)

ActionView::Template::Error (No route matches {:action=>"new", :controller=>"avo/drivers"}):
    1: <%= turbo_frame_tag "new_via_belongs_to" do %>
    2:   <%= render(Avo::ModalComponent.new(width: :xl, body_class: "bg-application")) do |c| %>
    3:     <div class="pt-4 pb-8">
    4:       <%= render Avo::Views::ResourceEditComponent.new(
    5:         resource: @resource,
    6:         model: @model,
    7:         view: @view,

@xeron
Copy link

xeron commented Oct 26, 2023

@sdcoffey could you please take a look? ^^ It doesn't open a modal for me, it just goes to the link and shows a "No route" error.

@adrianthedev
Copy link
Collaborator

@xeron was this fixed for you? I can't replicate it on my end.
If you can share a reproduction repository I'll gladly have a look.

@xeron
Copy link

xeron commented Oct 31, 2023

No, I tried to remove all my avo customizations:

  • custom avo stimulus controllers
  • custom avo _head and _scripts partials
  • custom avo routes

also tried downgrading rails from 7.1 to 7.0.

And it still didn't work. Clicking Create new resource just opens a page with a routing error for me.

I'll try to create a minimal app which reproduces the issue.

@adrianthedev
Copy link
Collaborator

Can you check if you have a JS error somewhere. To me that sounds like the JS is not working. Or Turbo doesn't execute it as a Turbo request.
That should be a turbo request.

@xeron
Copy link

xeron commented Oct 31, 2023

Can you check if you have a JS error somewhere. To me that sounds like the JS is not working. Or Turbo doesn't execute it as a Turbo request.

This was the 1st thing I did when I faced the issue :)

Anyway I think I found the problem.

Enabling the following in config/initializers/avo.rb breaks the functionality:

config.buttons_on_form_footers = true

@xeron
Copy link

xeron commented Nov 1, 2023

I've reported it as an issue for tracking purposes: #2008

@adrianthedev
Copy link
Collaborator

Pfff. What a weird bug!
Thanks for looking into this.
I'll add it to the roadmap.

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Nov 9, 2023

Can you check if you have a JS error somewhere. To me that sounds like the JS is not working. Or Turbo doesn't execute it as a Turbo request.

This was the 1st thing I did when I faced the issue :)

Anyway I think I found the problem.

Enabling the following in config/initializers/avo.rb breaks the functionality:

config.buttons_on_form_footers = true

Back button without valid path was rendering on the form footer, this PR fixes it. Thanks for pointing to the right point and for building a detailed issue!

@lpe
Copy link

lpe commented Nov 12, 2023

It would indeed be important to be able to disconnect this option, if possible at field level. For example I have "Task belongs_to User", I can pick a User (searchable). This certainly does not mean that I can create a User. Maybe there's another way to disable it? Thx.

@xeron
Copy link

xeron commented Nov 12, 2023

It would indeed be important to be able to disconnect this option

I was thinking about the same. This feature should be opt-in or opt-out in belongs_to field settings.

In fact I even had this in my code before:

field :driver, as: :belongs_to,
  help: 'You can add new drivers <a href="/admin/resources/drivers/new" target="_blank">here</a>',

So this feature automatically added duplicate functionality to some of my fields.

@adrianthedev
Copy link
Collaborator

Yup. That makes sense.
Apologies for the troubles.
We'll add a mechanism to hide that option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Create a belongs_to record from the associated record screen
6 participants